feat(database): support credentials rotation#707
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/database/prod/src/main.ts`:
- Around line 127-128: The NeonPool instance is being initialized with a static
connectionString value that won't refresh when credentials rotate, while
createRefreshingHttpClient correctly uses a Proxy pattern to refresh credentials
at each invocation via readConnectionString. Determine if this is intentional
behavior (since pool connections are long-lived and may not require credential
refresh) and if so, add clear comments above the NeonPool initialization
explaining this limitation and why direct pool usage won't benefit from
credential rotation. Alternatively, if the pool should support credential
rotation like httpClient does, refactor the NeonPool initialization to use a
similar lazy-refresh pattern with readConnectionString instead of the static
connectionString value from line 121.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d6c818d4-23b1-4ecd-9936-751869c77aac
📒 Files selected for processing (2)
packages/database/prod/src/main.test.tspackages/database/prod/src/main.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
netlify/blueprints(manual)
| return new Proxy<NeonHttpClient>(target, { | ||
| apply: (_target, _thisArg, args: unknown[]) => (getClient() as (...callArgs: unknown[]) => unknown)(...args), | ||
| get: (_target, prop) => { | ||
| const value = (getClient() as unknown as Record<string | symbol, unknown>)[prop] |
There was a problem hiding this comment.
Not sure if needed here, but Reflect.get(getClient(), prop) might be safer (ref https://stackoverflow.com/a/59706174), but wether it even matters depends on the client internals
There was a problem hiding this comment.
Yeah, I was on the fence about it. I think in this particular case it wouldn't make a difference because the underlying client only has plain methods (no getters). But perhaps using Reflect.get is more idiomatic and future-proof, in case the client grows into a more complex shape.
| const fn = (getClient() as unknown as Record<string | symbol, unknown>)[prop] as ( | ||
| ...callArgs: unknown[] | ||
| ) => unknown |
There was a problem hiding this comment.
Could this be just this?
| const fn = (getClient() as unknown as Record<string | symbol, unknown>)[prop] as ( | |
| ...callArgs: unknown[] | |
| ) => unknown | |
| const fn = value as ( | |
| ...callArgs: unknown[] | |
| ) => unknown |
Doesn't matter to me if it could - but just trying to gauge if I'm missing a reason that this should get a new reference for accessed prop
2 participants
Changes the Netlify Database client to lazily read the
NETLIFY_DB_URLenvironment variable. at query time so we can support credential rotation.